Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Prettier v3 #2947

Merged
merged 15 commits into from
Apr 21, 2023
Merged

Support Prettier v3 #2947

merged 15 commits into from
Apr 21, 2023

Conversation

sosukesuzuki
Copy link
Member

@sosukesuzuki sosukesuzuki commented Apr 6, 2023

  • Run tests
  • Update the CHANGELOG.md with a summary of your changes

Supports Prettier v3. Failed tests are not related to this PR. It also fails in main.

@sosukesuzuki
Copy link
Member Author

I found the problem of not being able to read ESM format configuration files...

@fisker
Copy link
Member

fisker commented Apr 11, 2023

I found the problem of not being able to read ESM format configuration files...

Prettier issue?

@sosukesuzuki
Copy link
Member Author

@fisker

Prettier issue?

Maybe No, and maybe it happens only with this test. When I actually ran it on VSCode, it worked fine.

@sosukesuzuki
Copy link
Member Author

I have found it difficult to test the loading of the configuration file with the way prettier-vscode testing now.
Even when testing the v3 folder, the configuration file is resolved using resolveConfigFile in Prettier 2.8.7. This is probably not a problem for users. It is a problem specific to testing this repository.

We will need to find a better way to test this in the future, for now I think we are good to go.

@sosukesuzuki sosukesuzuki requested a review from fisker April 15, 2023 11:21
src/PrettierEditService.ts Outdated Show resolved Hide resolved
fisker
fisker previously approved these changes Apr 15, 2023
Copy link
Member

@fisker fisker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm.

@fisker
Copy link
Member

fisker commented Apr 15, 2023

This one didn't awaited https://github.com/sosukesuzuki/prettier-vscode/blob/ab1b4ecfa540c634ddf2b2faaba1a60ab0a9ec57/src/ModuleResolver.ts#L327

I reviewed on cellphone, please double check Prettier api calls.

@fisker fisker dismissed their stale review April 15, 2023 19:20

Need change

src/PrettierEditService.ts Outdated Show resolved Hide resolved
@sosukesuzuki sosukesuzuki marked this pull request as ready for review April 16, 2023 08:08
@auto-assign auto-assign bot requested a review from ntotten April 16, 2023 08:08
@@ -4,6 +4,10 @@ All notable changes to the "prettier-vscode" extension will be documented in thi

<!-- Check [Keep a Changelog](https://keepachangelog.com/) for recommendations on how to structure this file. -->

## [9.12.0]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a major release. 10.0.0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should release v10 after Prettier releases v3? This PR is not breaking anything, just add support for v3 alpha.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ntotten I agree with fisker. If you too can agree with this, please merge and release.

Copy link
Member

@fisker fisker Apr 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems there is a release workflow. So I guess we can create a v9.12.0 tag to trigger release?

release:
runs-on: ubuntu-latest
needs: package
if: startsWith(github.ref, 'refs/tags/v')

But let's wait for ntotten to respond first.

@ntotten
Copy link
Member

ntotten commented Apr 17, 2023

Let me know if you need help on the tests.

@fisker
Copy link
Member

fisker commented Apr 23, 2023

@sosukesuzuki You are right, even after I fix the import() error, it still can't load config and plugins.

@sosukesuzuki
Copy link
Member Author

sosukesuzuki commented Apr 23, 2023

Oh... So we must find the way execute import expr in prettier vscode...

@fisker
Copy link
Member

fisker commented Apr 23, 2023

Maybe this will work? LinqLover/downstream-repository-mining@9f57b01

@fisker
Copy link
Member

fisker commented Apr 23, 2023

Change to

var prettierPromise = new Function("x", "return import(x)")("./index.mjs");
["ERROR" - 19:48:21] A dynamic import callback was not specified.
TypeError: A dynamic import callback was not specified.

@kachkaev

This comment was marked as off-topic.

@fisker

This comment was marked as off-topic.

@sosukesuzuki
Copy link
Member Author

sosukesuzuki commented Apr 23, 2023

@sosukesuzuki
Copy link
Member Author

This is my guess: the reason why ESLint's Flat Config works fine may be because it is the Language Server that is doing the dynamic import. The client may not be able to do dynamic imports.

@sosukesuzuki
Copy link
Member Author

We may need to adopt that same architecture... Separating the server that runs Prettier and client reflects values to the editor.

@fisker
Copy link
Member

fisker commented Apr 23, 2023

I also found an issue seems related to esm linked to vscode iteration plan, but looks like not useful.

opral/monorepo#452

@kachkaev
Copy link
Member

kachkaev commented Apr 23, 2023

I tried VS Code Insiders, but it did not help:

["INFO" - 16:38:35] Extension Name: esbenp.prettier-vscode.
["INFO" - 16:38:35] Extension Version: 9.12.0.
["ERROR" - 16:38:37] Error handling text editor change
["ERROR" - 16:38:37] A dynamic import callback was not specified.
TypeError: A dynamic import callback was not specified.
	at new NodeError (node:internal/errors:387:5)
	at importModuleDynamicallyCallback (node:internal/process/esm_loader:39:9)
	at Object.<anonymous> (/path/to/project/node_modules/.pnpm/[email protected]/node_modules/prettier/index.cjs:647:23)
	at u._compile (/Applications/Visual Studio Code - Insiders.app/Contents/Resources/app/out/vs/loader.js:4:1271)
	at Module._extensions..js (node:internal/modules/cjs/loader:1243:10)
	at Module.load (node:internal/modules/cjs/loader:1058:32)
	at Module._load (node:internal/modules/cjs/loader:893:12)
	at f._load (node:electron/js2c/asar_bundle:2:13330)
	at b._load (/Applications/Visual Studio Code - Insiders.app/Contents/Resources/app/out/vs/workbench/api/node/extensionHostProcess.js:124:29918)
VS Code Insiders
Version: 1.78.0-insider
Commit: b19017cea80b6157aa8214c984a70022e77526f2
Date: 2023-04-21T05:39:53.263Z (2 days ago)

@sosukesuzuki
Copy link
Member Author

I think we need to change prettier-vscode to a client/server architecture like a eslint-vscode for releasing Prettier v3.

@ntotten @fisker What do you think? Do you have an alternative plan?

@fisker
Copy link
Member

fisker commented Apr 26, 2023

I don't have other plan.

@sosukesuzuki
Copy link
Member Author

Starting April 29th, my university and work will have 9 days off, so I'll try to implement it there.

@sosukesuzuki
Copy link
Member Author

I'm working on https://github.com/sosukesuzuki/prettier-language-server. It will still take time. This is a more difficult task than I thought.

@liuxingbaoyu
Copy link

Is it possible for us to start a subprocess to handle it?
This will be a bit slower, but it shouldn't affect much.

@sosukesuzuki
Copy link
Member Author

Sorry, what do you mean?

@liuxingbaoyu
Copy link

microsoft/vscode#130367 (comment)
Like this one.
We start a new node process and run prettier inside it.

@sosukesuzuki
Copy link
Member Author

Oh.. I'll try it later. thank you

@liuxingbaoyu
Copy link

image
A new discovery!
worker_threads in vscode is accessible. We can try to take advantage of it!

@fisker
Copy link
Member

fisker commented Jun 3, 2023

worker_threads doesn't support chdir, should be careful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants